-
Notifications
You must be signed in to change notification settings - Fork 15
feat: add support for install deps #867
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
/run pipeline |
|
/run pipeline |
|
/run pipeline |
|
/run pipeline |
|
/run pipeline |
|
/run pipeline |
|
/run pipeline |
|
/run pipeline |
2 similar comments
|
/run pipeline |
|
/run pipeline |
ocofaigh
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Too much code duplication - lets sync up to see how we can handle here. Especially since this code would even be duplicated across multiple repos too. Perhaps time to leverage https://github.com/terraform-ibm-modules/common-bash-library
ocofaigh
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we have scripts/install-deps.sh and modules/kube-audit/scripts/install-deps.sh? We should only have 1 script that should be used by all the modules in this repo. If needed update the script to support only installing certain binaries if required
| count = var.install_dependencies ? 1 : 0 | ||
| # change trigger to run every time | ||
| triggers = { | ||
| build_number = timestamp() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need this to trigger every time. It only need to trigger if the null resource has to run again
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess thats not possible, if we set triggers to other null_resource blocks, the install script will run after there is a change in the other null_resource block and not before.
scripts/install-deps.sh
Outdated
| # Optional custom URL prefix for all binaries | ||
| CUSTOM_KUBECTL_URL="${CUSTOM_KUBECTL_URL:-}" | ||
| CUSTOM_JQ_URL="${CUSTOM_JQ_URL:-}" | ||
| CUSTOM_OC_URL="${CUSTOM_OC_URL:-}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not documented anywhere? We should probably list the environment variabl overrides in the variable descrption.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would also rename them to:
CUSTOM_KUBECTL_URL->KUBECTL_DOWNLOAD_URLCUSTOM_JQ_URL->JQ_DOWNLOAD_URL
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm just thinking that there might be authentication required for someones custom URL, but I guess its on them to make sure that handled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added the documentation to the main README
scripts/install-deps.sh
Outdated
| # Install: kubectl | ||
| ####################################### | ||
|
|
||
| # renovate: datasource=github-releases depName=kubernetes/kubernetes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you will need to add a custom renovate rule in renovate.json for this to work (copy the one in common-dev-asstes)
|
@ocofaigh , |
|
/run pipeline |
@Aashiq-J Yea but you can still have it reference the scripts folder in the root directory |
scripts/install-binaries.sh
Outdated
|
|
||
| # Optional custom URL prefix for all binaries | ||
| KUBECTL_DOWNLOAD_URL="${KUBECTL_DOWNLOAD_URL:-}" | ||
| JQ_DOWNLOAD_URL="${JQ_DOWNLOAD_URL:-}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not default these to the public URLs if not specified and use it for the download later in the script. Then you won't need the if/else logic
scripts/install-binaries.sh
Outdated
| BINARY=jq | ||
|
|
||
| if ! command -v jq >/dev/null 2>&1; then | ||
| echo "jq not found. Installing latest stable version locally..." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replace "latest stable version" with the value of JQ_VERSION
ocofaigh
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Aashiq-J Lets hold off running the pipeline until we agree on the approach here.
Description
Add script to install binaries which are required by the scripts in the module.
Release required?
x.x.X)x.X.x)X.x.x)Release notes content
Run the pipeline
If the CI pipeline doesn't run when you create the PR, the PR requires a user with GitHub collaborators access to run the pipeline.
Run the CI pipeline when the PR is ready for review and you expect tests to pass. Add a comment to the PR with the following text:
Checklist for reviewers
For mergers